Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cleaup previous widevine vmp PRs #2693

Merged
merged 1 commit into from
Jun 19, 2019
Merged

Cleaup previous widevine vmp PRs #2693

merged 1 commit into from
Jun 19, 2019

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Jun 14, 2019 •
edited
Loading

Remove/reduce unnecessary patches with our scripts.
Remove brave_enable_cdm_host_verification gn vars.

Issue: brave/brave-browser#4905
Related PR: brave/brave-browser#4909

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@simonhong simonhong requested review from bridiver and NejcZdovc June 14, 2019 02:49
@simonhong simonhong self-assigned this Jun 14, 2019
@simonhong simonhong added this to the 0.68.x - Nightly milestone Jun 14, 2019
@simonhong simonhong marked this pull request as ready for review June 14, 2019 02:49
@simonhong simonhong force-pushed the cleanup_widevine_pr branch 3 times, most recently from 6b93ba3 to 4539012 Compare June 14, 2019 06:59
@simonhong simonhong added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Jun 14, 2019
@simonhong
Copy link
Member Author

Set skip labels except Windows because all others are passed and only Win64 was failed due to installer signing. it was also unrelated with this PR.

@simonhong simonhong force-pushed the cleanup_widevine_pr branch from 4539012 to 64bf838 Compare June 14, 2019 14:29
bridiver
bridiver reviewed Jun 14, 2019
View reviewed changes
patches/chrome-tools-build-win-create_installer_archive.py.patch Outdated
+ shutil.copy(os.path.join(src_dir, 'brave.exe'), chrome_dir)
+ shutil.copy(os.path.join(src_dir, 'chrome.dll'), version_dir)
+ shutil.copy(os.path.join(src_dir, 'chrome_child.dll'), version_dir)
+ from create_installer_archive_helper import (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please put on one line, lint is not an issue for patches

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

bridiver
bridiver reviewed Jun 14, 2019
View reviewed changes
patches/chrome-tools-build-win-create_installer_archive.py.patch Outdated
version_numbers = prev_version.split('.')
prev_build_number = version_numbers[2] + '.' + version_numbers[3]

+ if not options.skip_signing:
+ from sign_binaries import sign_binaries
+ sign_binaries(staging_dir)
+ from create_installer_archive_helper import CopyPreSignedBinaries
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should go at https://github.com/brave/brave-core/pull/2693/files#diff-a9c9a8da7aa4df821394352a0ca04a27R9 with the others, but also we shouldn't be calling two methods here. We should have a single method for both sign_binaries and CopyPreSignedBinaries

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

bridiver
bridiver reviewed Jun 14, 2019
View reviewed changes
patches/chrome-tools-build-win-create_installer_archive.py.patch Outdated
+ from create_installer_archive_helper import (
+ CopyBraveExtensionLocalization,
+ CopyBraveRewardsExtensionLocalization)
+ CopyBraveExtensionLocalization(config, staging_dir, g_archive_inputs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two methods should be combined. There is no reason to call two methods in a row in patch that have the same params

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@simonhong simonhong force-pushed the cleanup_widevine_pr branch from 64bf838 to dfab44b Compare June 15, 2019 04:35
@simonhong
Copy link
Member Author

@bridiver Reduced to just 3 lines ;) PTAL again.

@simonhong
Copy link
Member Author

Also brave extension & rewards extension localization works properly.

@simonhong simonhong force-pushed the cleanup_widevine_pr branch from dfab44b to 2e62783 Compare June 15, 2019 04:41
@simonhong
Cleaup previous widevine vmp PRs
a6cd052 
Remove/reduce unnecessary patches with our scripts.
Remove brave_enable_cdm_host_verification gn vars.
@simonhong simonhong force-pushed the cleanup_widevine_pr branch from 2e62783 to a6cd052 Compare June 15, 2019 12:24
@bsclifton bsclifton mentioned this pull request Jun 17, 2019
Closed
2 tasks
@simonhong simonhong merged commit 34c4f06 into master Jun 19, 2019
@simonhong simonhong deleted the cleanup_widevine_pr branch June 19, 2019 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver approved these changes

@NejcZdovc NejcZdovc Awaiting requested review from NejcZdovc

Assignees

@simonhong simonhong

Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64
Projects
None yet
Milestone
0.68.x - Release
Development

Successfully merging this pull request may close these issues.
2 participants
@simonhong @bridiver